Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#97 Implemented delayed validation by introducing delayedValidation o… #119

Merged
merged 3 commits into from
Aug 16, 2024

Conversation

andkorsh
Copy link

@andkorsh andkorsh commented Aug 8, 2024

#97 Implemented delayed validation by introducing delayedValidation option.

The logic is a little bit more complicated than switching validation event to blur as initially discussed in the issue. This implementation doesn't do any validation while the user typing for the first time (i.e. until the first 'change' or 'submit') but once the validation had failed and user focused back into the input, validation switches back to "input" event to remove the error as soon as the data is valid.

src/index.ts Outdated
!input.classList.contains(this.ValidationInputCssClassName)
) {
// When delayedValidation=true, "input" only takes it back to valid. "Change" can make it invalid.
return;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to return true or false.

@haacked
Copy link
Owner

haacked commented Aug 9, 2024

Do you mind adding a sample page with delayed validation enabled so it's easy to test the behavior?

@andkorsh
Copy link
Author

andkorsh commented Aug 9, 2024

@haacked sure, thanks for looking at this. Fixed the return statement and added a sample. Please have a look when the time allows. We'd love to see it in the package to use it in our app.

Also below is a gif demonstrating the behavior of new delayed validation. Idea is, it doesn't bother you while you still typing, but only shows up when you finished and focused out/submitted. And if the field is errored and you are back in it to fix it, it will turn back valid as soon as the data becomes valid.

delayed_validation

In fact, the original lib at https://jqueryvalidation.org/files/demo/ behaves very similarly:

delayed_validation (original)

@haacked
Copy link
Owner

haacked commented Aug 13, 2024

Oh yeah, the current behavior is annoying. It seems to me the behavior in this PR should be the default as it seems to align with the jquery validation behavior. @dahlbyk any thoughts?

@andkorsh
Copy link
Author

@haacked If we choose to make this behavior default, I think it would be better to remove the delayedValidation option altogether, leaving users with data-val-event="input" should they require the old behavior. Also we'd need to refactor the below lines and get rid of nested ternary. When I implemented this PR, I tried not to touch any of the existing behaviors and did not fix the bug where data-val-event value isn't considered at all. But if we do choose to introduce a change in default behavior, fixing the bug would be logical.

const validateEvent = input.dataset.valEvent || input instanceof HTMLSelectElement ? 'change' :
(this.options.delayedValidation ? 'input change' : 'input');
const events = validateEvent.split(' ');

Please let me know what you decide and I can take care of it.

cheers,
Andrew

@haacked
Copy link
Owner

haacked commented Aug 15, 2024

@andkorsh I agree with your plan. Let’s make it so.

…nts. Fixed a bug with data-val-event not being respected
@andkorsh
Copy link
Author

@haacked I have updated the PR now and created a "Immediate Validation" sample page that comparing the new default behavior with the old behavior which can now be achieved using data-val-event="input".

cheers!

Copy link
Owner

@haacked haacked left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I need to test it before I approve it.

Comment on lines +1072 to +1076
if (
!input.dataset.valEvent &&
event && event.type === 'input' &&
!input.classList.contains(this.ValidationInputCssClassName)
) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit, but I prefer the logical operators in a multi-line conditional to align on the left. Makes it easier to see all the component conditionals.

Suggested change
if (
!input.dataset.valEvent &&
event && event.type === 'input' &&
!input.classList.contains(this.ValidationInputCssClassName)
) {
if (!input.dataset.valEvent
&& event
&& event.type === 'input'
&& !input.classList.contains(this.ValidationInputCssClassName)
) {

@haacked haacked merged commit 746a8a9 into haacked:main Aug 16, 2024
1 check passed
@andkorsh
Copy link
Author

Thank you! And appreciate you creating this great package, hope Microsoft will use it and remove the jquery dependency from identity templates one day.

@lonix1
Copy link
Contributor

lonix1 commented Aug 17, 2024

@andkorsh As discussed in original issue, is the final result still a drop-in replacement? If not, what should I change in my code to upgrade to these bits?

@andkorsh
Copy link
Author

andkorsh commented Aug 17, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants